metrics_utility/test/library/test_summarize_merge_ops.py (2)

57-91: Merge tests pass but don't exercise many-to-many semantics.

Both tests use disjoint single-row indexes, which is effectively a one-to-one outer join — they confirm merge() doesn't crash and returns 2 rows but don't actually validate the many-to-many path. Fine to keep as a smoke test for the SonarCloud fix; just noting that duplicate keys on both sides would more directly cover the validate parameter behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/test/library/test_summarize_merge_ops.py` around lines 57 -
91, Update the two tests (test_base_traditional_merge_many_to_many and
test_base_engine_merge_many_to_many) so they use duplicate keys on both sides to
actually exercise many-to-many semantics: build df1 and df2 with repeated 'id'
values (e.g., id duplicated) so the merge triggers the many-to-many path in
Base.merge()/ _ConcreteTraditional.merge and assert the expected expanded row
count (e.g., 4 for two duplicates on each side) instead of using disjoint
single-row indexes.

---

30-33: Weak assertion for combine_json_values.

The test only verifies key 'k' is present, not that values from both inputs are actually combined. Given combine_json_values builds a set per key, a stronger assertion exercises the real behavior:

♻️ Proposed stronger assertion

-def test_summarize_combine_json_values():
-    df = pd.DataFrame({'col_x': [{'k': 'v1'}], 'col_y': [{'k': 'v2'}]})
-    result = _bt().summarize_merged_dataframes(df, ['col'], operations={'col': 'combine_json_values'})
-    assert 'k' in result['col'].iloc[0]
+def test_summarize_combine_json_values():
+    df = pd.DataFrame({'col_x': [{'k': 'v1'}], 'col_y': [{'k': 'v2'}]})
+    result = _bt().summarize_merged_dataframes(df, ['col'], operations={'col': 'combine_json_values'})
+    assert result['col'].iloc[0] == {'k': {'v1', 'v2'}}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/test/library/test_summarize_merge_ops.py` around lines 30 -
33, The test test_summarize_combine_json_values has a weak assertion that only
checks the presence of key 'k' but not that values from both input dicts were
combined; update the assertion on the result of
_bt().summarize_merged_dataframes (operation 'combine_json_values') to assert
that the value for 'k' contains both 'v1' and 'v2' (e.g., that the set or list
at result['col'].iloc[0]['k'] equals or includes both values) so the test
verifies the actual combining behavior.

metrics_utility/library/dataframes/base_traditional.py (1)

14-14: Mixin defaults silently return empty schemas.

DataframeSchemaMixin returns {}/[] for all schema methods, so any subclass of BaseTraditional that forgets to override (e.g.) unique_index_columns() will silently merge/cast on empty keys and produce wrong rollups instead of failing fast. The previous local placeholders presumably had the same property, so this isn't a regression — but worth either documenting the contract on the mixin or having the defaults raise NotImplementedError (or using abc.abstractmethod) to surface mistakes early.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/library/dataframes/base_traditional.py` at line 14,
DataframeSchemaMixin currently returns empty dicts/lists which lets subclasses
like BaseTraditional silently use empty schemas; change the mixin so its schema
accessor methods (e.g., unique_index_columns, index_columns, schema_fields —
whatever schema methods exist on DataframeSchemaMixin) either raise
NotImplementedError by default or, better, mark them as abstract using
abc.abstractmethod so concrete subclasses must implement them; update
DataframeSchemaMixin accordingly and ensure BaseTraditional (and its subclasses)
implement the required methods to avoid silent empty-schema behavior.

metrics_utility/automation_controller_billing/report_saver/report_saver_s3.py (1)

19-19: Avoid materializing entire iterable just to check non-emptiness.

If list_files returns an iterator/generator over potentially many S3 objects, list(...) walks all of them. Using any() short-circuits on the first item.

♻️ Proposed refactor

-        return len(list(self.s3_handler.list_files(self.report_spreadsheet_destination_path))) > 0
+        return any(True for _ in self.s3_handler.list_files(self.report_spreadsheet_destination_path))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@metrics_utility/automation_controller_billing/report_saver/report_saver_s3.py`
at line 19, The current check materializes the entire iterator by doing
len(list(self.s3_handler.list_files(self.report_spreadsheet_destination_path)))
> 0; replace this with a short-circuiting check using
any(self.s3_handler.list_files(self.report_spreadsheet_destination_path)) so the
generator/iterator returned by list_files is not fully consumed (update the
return expression in the method that contains this line).

metrics_utility/test/library/test_storage_s3.py (1)

92-94: LGTM — covers the new ValueError contract.

Asserting both the exception type and match='bucket not set' pins down the public failure mode of StorageS3() without a bucket.

One small consideration: the message in s3.py is 'StorageS3: bucket not set' while other tests in the suite (e.g. test_storage_directory.py) likely match the prefixed form. Worth verifying the match regex is the intended substring across the storage test family for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/test/library/test_storage_s3.py` around lines 92 - 94, The
test test_missing_bucket_raises_value_error asserts the error message is 'bucket
not set' but the implementation in s3.py raises "StorageS3: bucket not set",
causing an inconsistency with other storage tests; update the
test_missing_bucket_raises_value_error to match the actual prefixed message
(e.g., use match='StorageS3: bucket not set' or a regex that accepts the prefix)
or alternatively change the raise message in StorageS3 (in s3.py) to the
unprefixed form—ensure consistency across StorageS3, the test function
test_missing_bucket_raises_value_error, and other storage tests like
test_storage_directory.py.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

metrics_utility/test/base/functional/helpers.py (1)

64-67: Inconsistent with the S7508 cleanup applied elsewhere.

The same in files.keys() idiom was simplified throughout test_gathering.py and test_slicing.py in this PR but left here. For consistency:

♻️ Proposed cleanup

 def assert_common_files(files):
-    assert './config.json' in files.keys()
-    assert './manifest.json' in files.keys()
-    assert './data_collection_status.csv' in files.keys()
+    assert './config.json' in files
+    assert './manifest.json' in files
+    assert './data_collection_status.csv' in files

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/test/base/functional/helpers.py` around lines 64 - 67, In
assert_common_files, remove the unnecessary .keys() calls to match the S7508
cleanup elsewhere: update the three assertions in the assert_common_files
function to check membership directly in the mapping (e.g., assert
'./config.json' in files) instead of using in files.keys(); this keeps
consistency with test_gathering.py and test_slicing.py.

metrics_utility/library/utils.py (1)

30-37: No positional callers found; however, consider preserving explicit parameter names for the public API.

The search confirms all repository callers use keyword arguments (db=, key=, value=), so the **_kwargs signature is safe internally. Both functions are exported in library/__init__.py as public API. While the current change introduces no breakage within the repo, external consumers relying on the explicit signature could be affected. For a public API, preserving the parameter contract improves discoverability and reduces surprise:

♻️ Suggested alternative preserving the signature

-def last_gather(**_kwargs):
+def last_gather(_db=None, _key=None):
     log('library.utils last_gather')
     return None


-def save_last_gather(**_kwargs):
+def save_last_gather(_db=None, _key=None, _value=None):
     log('library.utils save_last_gather')

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/library/utils.py` around lines 30 - 37, The public functions
last_gather and save_last_gather were changed to accept only **_kwargs which can
surprise external callers; restore explicit parameter names in their signatures
(e.g., def last_gather(db=None, key=None, value=None, **_kwargs) and def
save_last_gather(db=None, key=None, value=None, **_kwargs)) so the public API
documents expected parameters while still allowing forward-compatible keyword
captures; update the function docstrings/comments if present and keep the
internal logging behavior intact (these functions are exported via
library.__init__ as public API).

metrics_utility/automation_controller_billing/report_saver/report_saver_s3.py (1)

18-19: Prefer any(...) over len(list(...)) > 0 to avoid materializing all keys.

s3_handler.list_files is a generator that paginates over S3 (paginator.paginate(...)); wrapping it in list(...) forces enumeration of every object under the prefix just to check existence, which is wasted I/O and memory when many files exist. Use any(...) to short-circuit on the first hit.

Proposed fix

-    def report_exist(self):
-        return len(list(self.s3_handler.list_files(self.report_spreadsheet_destination_path))) > 0
+    def report_exist(self):
+        return any(self.s3_handler.list_files(self.report_spreadsheet_destination_path))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@metrics_utility/automation_controller_billing/report_saver/report_saver_s3.py`
around lines 18 - 19, The report_exist method currently materializes all keys by
doing
len(list(self.s3_handler.list_files(self.report_spreadsheet_destination_path)))
> 0; change it to use a short-circuiting check with any(...) over the generator
returned by self.s3_handler.list_files(self.report_spreadsheet_destination_path)
so it returns True on the first found object and avoids building a full list.

metrics_utility/test/base/test_package.py (1)

11-23: Optional: prefer a real Package instance for has_free_space tests.

Calling Package.has_free_space(pkg, …) with a MagicMock(spec=Package) works today, but it bypasses the actual get_max_data_size classmethod and only exercises the inequality. Constructing a real Package(collector=Mock()) (or a thin subclass overriding MAX_DATA_SIZE) would also catch regressions in get_max_data_size itself.

Suggested approach

-def test_has_free_space_within_limit():
-    pkg = MagicMock(spec=Package)
-    pkg.total_data_size = 100
-    pkg.get_max_data_size.return_value = 200
-    assert Package.has_free_space(pkg, 99) is True
-    assert Package.has_free_space(pkg, 100) is True  # exactly at limit
+def test_has_free_space_within_limit(monkeypatch):
+    pkg = Package(collector=MagicMock())
+    pkg.total_data_size = 100
+    monkeypatch.setattr(Package, 'MAX_DATA_SIZE', 200)
+    assert pkg.has_free_space(99) is True
+    assert pkg.has_free_space(100) is True  # exactly at limit

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/test/base/test_package.py` around lines 11 - 23, Tests use
MagicMock(spec=Package) which bypasses Package.get_max_data_size behavior;
replace the MagicMock-based pkg with a real Package instance (e.g.,
Package(collector=Mock()) or a small test subclass that sets MAX_DATA_SIZE) so
that Package.has_free_space is exercised against the actual get_max_data_size
logic in tests test_has_free_space_within_limit and
test_has_free_space_exceeds_limit; ensure you set pkg.total_data_size
appropriately and call Package.has_free_space(pkg, ...) as before.

metrics_utility/anonymized_rollups/anonymized_rollups.py (1)

278-278: Optional: drop the redundant ternary.

sorted(empty_set) already returns [], so the if ansible_versions_set else [] guard is unnecessary.

♻️ Proposed simplification

-    return sorted(ansible_versions_set) if ansible_versions_set else []
+    return sorted(ansible_versions_set)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/anonymized_rollups/anonymized_rollups.py` at line 278, The
return expression currently uses a redundant ternary: "return
sorted(ansible_versions_set) if ansible_versions_set else []"; simplify it by
returning sorted(ansible_versions_set) directly since sorted(empty_set) already
yields [], i.e., replace that return with "return sorted(ansible_versions_set)"
referencing the ansible_versions_set variable and the existing sorted call.

metrics_utility/library/collectors/util.py (1)

22-23: TypeError is the right exception, but consider isinstance for subclass tolerance.

type(x) is not dict / type(x) is not list rejects valid subclasses (e.g., OrderedDict, collections.UserList). If you want to keep the exact-type semantics, fine — otherwise prefer isinstance for forward-compatibility.

♻️ Optional refactor

-        if type(data) is not dict:
+        if not isinstance(data, dict):
             raise TypeError('data must be a dict, or None')

-        if type(filenames) is not list:
+        if not isinstance(filenames, list):
             raise TypeError('filenames must be a list, or None')

Also applies to: 40-41

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/library/collectors/util.py` around lines 22 - 23, Replace
strict type checks that use "type(x) is not dict" and "type(x) is not list" with
isinstance-based checks so subclasses are accepted; specifically update the
conditional guarding the variable "data" (currently using type(data) is not
dict) to use "isinstance(data, dict)" and likewise update the check around the
list variable (the check at lines ~40-41) to use isinstance(..., list) or, if
more appropriate, collections.abc.Sequence/UserList depending on intended
semantics, preserving the existing TypeError and error message text.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes